fix(sdk): improvements in default sandbox.write and sandbox.read implementations#2321
fix(sdk): improvements in default sandbox.write and sandbox.read implementations#2321Eugene Yurtsev (eyurtsev) merged 12 commits intomainfrom
Conversation
github-actions
bot
added
deepagents
Eugene Yurtsev (eyurtsev)
changed the title
Merging this PR will not alter performance
Comparing 
Footnotes
|
Eugene Yurtsev (eyurtsev)
changed the title
There was a problem hiding this comment.
Pull request overview
This PR updates the sandbox backend to reduce overly-defensive exception handling in write(), and improves read() behavior to better handle large outputs returned via execute().
Changes:
- Update
BaseSandbox.read()’s server-side script to cap text output size and add truncation guidance; add binary-preview size limits. - Simplify
BaseSandbox.write()by letting backend exceptions bubble up and changing behavior whenupload_files()returns an empty response. - Update/adjust unit tests to cover truncated read output and align write success assertions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| libs/deepagents/deepagents/backends/sandbox.py | Adjusts the server-side read script (truncation + binary handling) and changes write/upload error handling + docs. |
| libs/deepagents/tests/unit_tests/backends/test_sandbox_backend.py | Adds coverage for truncated read content and updates write success expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except UnicodeDecodeError: | ||
| with open(path, 'rb') as f: | ||
| raw = f.read() | ||
| print(json.dumps({{'encoding': 'base64', 'content': base64.b64encode(raw).decode('ascii')}})) | ||
| sys.exit(0) |
There was a problem hiding this comment.
In the non-UTF-8 fallback, the script reads and base64-encodes the entire file without applying the MAX_BINARY_BYTES guard. Since _get_file_type() defaults to text for unknown extensions, large binary files can hit this path and exceed stdout/transport limits. Consider checking os.path.getsize(path) against MAX_BINARY_BYTES before f.read() here (and returning a clear error) to keep the size cap effective for all binary outputs.
| if returned_lines == 0 and not truncated: | ||
| print(json.dumps({{'error': 'Line offset ' + str(offset) + ' exceeds file length (' + str(line_count) + ' lines)'}})) | ||
| sys.exit(0) |
There was a problem hiding this comment.
The returned_lines == 0 check currently reports an "offset exceeds file length" error even when limit == 0 (the loop exits immediately). That makes limit=0 behave like an invalid offset. Consider handling limit <= 0 explicitly (e.g., return empty content) and/or include limit in the error condition so offset errors only occur when offset is actually beyond EOF.
| # An unreachable condition was reached | ||
| msg = f"Responses was expected to return 1 result, but it returned {len(responses)} with type {type(responses)}" | ||
| raise AssertionError(msg) |
There was a problem hiding this comment.
write() now raises an AssertionError when upload_files() returns an empty list. This can crash callers in production for what is effectively an integration/backend contract violation, and it also removes the prior structured WriteResult(error=...) behavior. Consider returning a WriteResult error (or raising a dedicated exception type) with actionable context, rather than asserting.
| # An unreachable condition was reached | |
| msg = f"Responses was expected to return 1 result, but it returned {len(responses)} with type {type(responses)}" | |
| raise AssertionError(msg) | |
| msg = ( | |
| f"Failed to write file '{file_path}': upload_files() returned no responses " | |
| f"for 1 requested upload (got {len(responses)}; type={type(responses).__name__})" | |
| ) | |
| return WriteResult(error=msg) |
| Implementations must support partial success - catch exceptions per-file | ||
| and return errors in `FileUploadResponse` objects rather than raising. | ||
|
|
||
| Upload files is responsible for ensuring that the parent path exists | ||
| (if user permissions allow the user to write to the given directory) |
There was a problem hiding this comment.
This docstring says upload_files() is responsible for ensuring the parent path exists, but BaseSandbox.write() still runs _WRITE_CHECK_TEMPLATE which calls os.makedirs(...) (and the comment above write() still says "Existance check + mkdir"). Either move the mkdir responsibility fully into upload_files() as stated, or update the docs/comments/PR description to reflect the actual behavior to avoid double work and confusion.
Uh oh!
There was an error while loading. Please reload this page.